Add support for generated identifier format strings (id prefixing). #509
Conversation
WalkthroughPublic ID-generator signatures gained an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/orm/client-api/generated-id-format-strings.test.ts (2)
7-1060: Consider refactoring verbose schema definition.The schema definition spans over 1000 lines with significant duplication—each field's default value is defined identically in both
attributes[0].args[0].valueand thedefaultproperty. While this may be required by theSchemaDeftype structure, consider:
- Extract a helper function to generate ID field definitions:
function makeIdField(name: string, funcName: string, args: any[], format: string) { const callExpr = { kind: 'call', function: funcName, args }; return { name, type: 'String', attributes: [{ name: '@default', args: [{ name: 'value', value: callExpr }] }], default: callExpr }; }
- This would reduce the ~1050-line schema to ~100-200 lines, improving readability and maintainability.
1062-1167: Solid E2E coverage for core scenarios.The tests effectively validate ID formatting across top-level, nested, and deeply nested creation scenarios, covering all supported ID generation strategies (uuid, uuid7, cuid, cuid2, nanoid, nanoid8, ulid).
Consider adding test cases for edge cases:
- Invalid format strings (missing "%s") to verify validation errors
- Format strings with special characters or unusual patterns
- Verification that the generated suffix is a valid ID (not just the prefix)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/language/res/stdlib.zmodel(2 hunks)packages/orm/src/client/crud/operations/base.ts(2 hunks)tests/e2e/orm/client-api/generated-id-format-strings.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zmodel: Always runzenstack generateafter modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript viazenstack generate
Files:
packages/language/res/stdlib.zmodel
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
E2E tests should validate real-world schema compatibility with established projects
Files:
tests/e2e/orm/client-api/generated-id-format-strings.test.ts
🧠 Learnings (7)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
tests/e2e/orm/client-api/generated-id-format-strings.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests
Applied to files:
tests/e2e/orm/client-api/generated-id-format-strings.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
tests/e2e/orm/client-api/generated-id-format-strings.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
tests/e2e/orm/client-api/generated-id-format-strings.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
tests/e2e/orm/client-api/generated-id-format-strings.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
tests/e2e/orm/client-api/generated-id-format-strings.test.ts
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
Applied to files:
tests/e2e/orm/client-api/generated-id-format-strings.test.ts
🧬 Code graph analysis (1)
packages/orm/src/client/crud/operations/base.ts (2)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (4)
packages/language/res/stdlib.zmodel (1)
86-105: LGTM! Consistent API extension for ID formatting.The optional
formatparameter has been consistently added to all ID generation functions (uuid, cuid, nanoid, ulid), maintaining backward compatibility while enabling the new prefixing feature.packages/orm/src/client/crud/operations/base.ts (3)
863-878: LGTM! Consistent integration of format string support.The formatGeneratedValue helper is correctly integrated into all ID generators:
- cuid, uuid, nanoid use args[1] for the format parameter (after version/length)
- ulid correctly uses args[0] since it has no version parameter
The logic properly handles optional formatting while maintaining backward compatibility.
896-912: Clarify single vs. multiple placeholder handling.The implementation replaces only the first occurrence of "%s" using
replace(). The comment at line 910 questions whetherreplaceAllshould be used instead.Consider:
- Should multiple "%s" placeholders be supported (e.g.,
"prefix_%s_suffix_%s")?- If multiple placeholders aren't intended, should the validation enforce a single "%s"?
- The current behavior will leave subsequent "%s" as literal strings, which may be unexpected.
Additionally, consider adding type validation that
generatedis a string before callingreplace(), even though the generators should always return strings.For design clarity, please confirm the intended behavior and consider either:
- Using
replaceAll('%s', generated)if multiple placeholders should be supported- Validating exactly one "%s" exists if single replacement is intended
907-908: Effective input validation.The invariant checks properly validate that the format is a string and contains the required "%s" placeholder, providing clear error messages if the format is misconfigured.
|
Looking into the failed tests. |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/language/res/stdlib.zmodel(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.zmodel
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.zmodel: Always runzenstack generateafter modifying ZModel schemas
ZModel schema files should define database structure and policies that compile to TypeScript viazenstack generate
Files:
packages/language/res/stdlib.zmodel
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/orm/src/client/crud/operations/base.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/orm/src/client/crud/operations/base.ts (2)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (1)
packages/orm/src/client/crud/operations/base.ts (1)
863-890: Consider explicit version validation for UUID generation.The implementation handles UUID v7 generation correctly, but silently defaults all other values (including v1, v3, v5, v6, or invalid inputs) to v4. Since the uuid library supports versions 1, 3, 4, 5, 6, and 7, consider either:
- Supporting all available versions
- Validating the version parameter and throwing an error for unsupported values
- Explicitly documenting that only v7 and v4 are supported
|
@ymc9 tests are passing and this should be good to go. I debated between Let me know what you think and if you'd like more test cases. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/orm/src/client/crud/operations/base.ts (1)
904-910: Add defensive runtime checks for format string validity.While language-level validation ensures format strings contain
%s, the runtime formatter lacks defensive checks. If the validator is bypassed or the schema is inconsistent, a format string without%swould produce non-unique IDs.Additionally, the current implementation uses
replaceAll, which substitutes all occurrences of%swith the same generated ID. Based on the PR examples ('user_uuid7_%s'), single substitution appears to be the primary use case. If multiple%splaceholders are unintended, consider usingreplacefor single substitution or documenting the multiple-substitution behavior.🔎 Suggested defensive checks
private formatGeneratedValue(generated: string, formatExpr?: Expression) { if (!formatExpr || !ExpressionUtils.isLiteral(formatExpr)) { return generated; } + + const format = formatExpr.value; + invariant(typeof format === 'string', 'generated identifier format value must be a string'); + invariant(format.includes('%s'), 'generated identifier format strings must include "%s"'); - return formatExpr.value.replaceAll('%s', generated); + // Use replace for single substitution, or document if multiple substitutions are intended + return format.replace('%s', generated); }
🧹 Nitpick comments (1)
packages/language/src/validators/function-invocation-validator.ts (1)
90-98: LGTM with an optional clarity suggestion.The validation logic correctly enforces that format strings include
%s. The code safely handles cases where the format parameter doesn't exist (formatParamIdx === -1) through optional chaining.💡 Optional refactor for clarity
Consider making the index check explicit to improve readability:
if (['uuid', 'ulid', 'cuid', 'nanoid'].includes(funcDecl.name)) { const formatParamIdx = funcDecl.params.findIndex(param => param.name === 'format'); - const formatArg = getLiteral<string>(expr.args[formatParamIdx]?.value); - if (formatArg && !formatArg.includes('%s')) { - accept('error', 'argument must include "%s"', { - node: expr.args[formatParamIdx]!, - }); + if (formatParamIdx >= 0) { + const formatArg = getLiteral<string>(expr.args[formatParamIdx]?.value); + if (formatArg && !formatArg.includes('%s')) { + accept('error', 'argument must include "%s"', { + node: expr.args[formatParamIdx], + }); + } } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/language/src/validators/function-invocation-validator.ts(1 hunks)packages/language/test/function-invocation.test.ts(1 hunks)packages/orm/src/client/crud/operations/base.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects
Applied to files:
packages/language/test/function-invocation.test.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/**/type*.{ts,tsx} : Ensure TypeScript inference and type coverage are validated through type coverage tests
Applied to files:
packages/language/test/function-invocation.test.ts
🧬 Code graph analysis (2)
packages/language/src/validators/function-invocation-validator.ts (1)
packages/language/src/utils.ts (1)
getLiteral(272-286)
packages/orm/src/client/crud/operations/base.ts (1)
packages/schema/src/expression-utils.ts (1)
ExpressionUtils(19-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (2)
packages/language/test/function-invocation.test.ts (1)
1-209: LGTM! Comprehensive test coverage.The test suite thoroughly validates the new format string functionality for ID generation functions:
- Confirms format strings are optional (backward compatibility)
- Validates correct handling of format strings containing
%s- Ensures validation errors for format strings missing
%sAll four ID generators (uuid, cuid, nanoid, ulid) are tested with various parameter combinations.
packages/orm/src/client/crud/operations/base.ts (1)
863-886: ID generation logic is correct.The modifications properly extract optional parameters (version, length) and pass format expressions to the new formatting helper. Parameter positions align with the extended function signatures.
1 participant
Sample Usage
There are arguments to be made that
@defaultcould be a better place to handle this, which would open up opportunities for default values to be composed of complex expressions.For now, this seems OK.
Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.